-
Notifications
You must be signed in to change notification settings - Fork 813
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move terraform modules from ./build to ./install #1167
Move terraform modules from ./build to ./install #1167
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
@amitlevy21 Please include also this file updated:
|
5e47ba1
to
49f1030
Compare
Build Failed 😱 Build Id: eb19ae27-d4c2-473d-94fc-3c20e12664fe To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Build Failed 😱 Build Id: 6acfe9c2-f052-4b92-8013-511734a34ce6 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
I will verify that all is working as it should. |
I performed
|
Your forgot to move all
We have a policy - one commit per Pull Request. |
I have created a commit which contains all updates necessary to conclude this PR: |
I think this is going to break of terraform installation instructions. The terraform module linked from here is https://github.com/googleforgames/agones/blob/release-1.1.0/examples/terraform-submodules/gke/module.tf Even though that file is on the 1.1 release branch, it pulls from the build directory on the master branch: If we move files out of that directory on the master branch, then our terraform instructions for all previous releases will break. If we want to move the files, we should copy them and leave them in the build directory as well (maybe a symlink would work?). We should also consider updating the gke/module.tf file to reference the release branch so that the configs are all versioned together. |
49f1030
to
e1535e3
Compare
@aLekSer @roberthbailey Thanks for the help. Regarding the reference to release branch in gke/module.tf, since release branches change frequently, how can I avoid hard coding a specific release branch? Is there a way to auto detect which release branch it should reference? |
Build Succeeded 👏 Build Id: 13e39d4f-0597-43cf-86fc-f9bdef20164e The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
@markmandel ^^ As far as I know, we have two ways of doing this in the codebase. Within the website, we have variable that get automatically substituted in for the release branch number but I don't think that magic happens outside of the website directory so unless I'm missing something you can't do something like Elsewhere, we have hard-coded numbers, along with a checklist when creating a release to bump the numbers. This is generally within the install directory (e.g. for the helm chart version) and since terraform is about installing, it might make sense to move the module.tf into the install directory and update the release checklist to bump the version number each time we create a release. |
Possible side thought on this - should the terraform install files also be included as a release artifact? Would that solve this issue (maybe in the long term, rather than short) |
e1535e3
to
bd5d006
Compare
@roberthbailey Rebased with #1179. I was unsure if I should update |
Build Failed 😱 Build Id: 30b047fe-b1d0-4abf-ad41-a766c6ba212e To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
Yes, it will be part of the 1.2 release. Edit: It looks like our best bet is to leave this at the master branch for now and then during the 1.2 release we will update it to point to the release branch. |
a46e489
to
123ce6b
Compare
Build Succeeded 👏 Build Id: d2b503aa-ac37-417c-a2bc-17b4ea898929 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
@aLekSer @roberthbailey Is there anything else needed here? Do I need to rebase again so the branch aligns with master? |
Yes, please update, other than that it looks good from my side. |
123ce6b
to
b5f263a
Compare
Build Succeeded 👏 Build Id: 3685f58a-14dc-4666-ab89-e013f4d62366 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Done |
Just confirming - this is good to approve and merge? if so, happy to hit the button. |
May I ask, what's holding this? |
I think it was waiting for a final lgtm from @aLekSer |
2d5b0c0
to
5bddc30
Compare
Build Succeeded 👏 Build Id: aab0f9ae-7add-40dd-af59-eddeff423e47 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Build Succeeded 👏 Build Id: 0d7622b2-148f-4821-9fb2-dee86847294a The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
LGTM |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: amitlevy21, markmandel The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
New changes are detected. LGTM label has been removed. |
Build Succeeded 👏 Build Id: 20cd5f0b-fe5e-4dce-b12f-a2757227a35d The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
Closes #1150.
@aLekSer Waiting for feedback on:
make terraform-init
make gcloud-terraform-cluster
make gcloud-terraform-install
make gcloud-terraform-destroy-cluster
Thanks!